Skip to content

Wkd.lookupEmail returns several keys#3561

Merged
tomholub merged 13 commits intomasterfrom
issue-3018-multiple-wkd-response
May 8, 2021
Merged

Wkd.lookupEmail returns several keys#3561
tomholub merged 13 commits intomasterfrom
issue-3018-multiple-wkd-response

Conversation

@rrrooommmaaa
Copy link
Contributor

@rrrooommmaaa rrrooommmaaa commented Apr 14, 2021

This PR processes multiple keys from Wkd and some other services

close #3018


Tests (delete all except exactly one):

  • Tests added or updated TODO:

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Adding some comments.

Comment on lines 102 to 103
const pub = await KeyUtil.parse(keyserverRes.pubkeys[0]); // todo: ?
this.view.acctEmailAttesterPubId = pub.id;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - please make acctEmailAttesterPubId an array. Then somewhere here will be acctEmailAttesterPubId !== prv.id or something similar, to see if the public key is consistent. Please change that to check if acctEmailAttesterPubId.includes(prv.id).

curve: (algoInfo as any).curve as string | undefined,
algorithmId: opgp.enums.publicKey[algoInfo.algorithm]
},
revoked: keyWithoutWeakPackets.revocationSignatures.length > 0
Copy link
Contributor Author

@rrrooommmaaa rrrooommmaaa Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the flag I was talking about. It's easily calculated when creating the Key structure.
So we don't really need to keep it in the database, do we?
The main question is: how should we prevent an update operation which replaces a revoked key with its unrevoked (outdated) version?

Copy link
Collaborator

@tomholub tomholub Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the flag I was talking about. It's easily calculated when creating the Key structure.
So we don't really need to keep it in the database, do we?

Ah. My intuition was to update the schema to add this flag to storage. Then filter based on that when pulling keys out of storage.

What you say is that you pull all keys for that user, then parse them and choose the appropriate key to use based on the parsed information. Your approach is simpler, and therefore better.

The main question is: how should we prevent an update operation which replaces a revoked key with its unrevoked (outdated) version?

I tend to do this in two ways - once in high level code and once more - a last resort hard stop - in low level code:

  1. a sensible logic preventing unwanted behavior in high-level code, in particular:
  • when pulling from pubkey lookup (compose)
  • when refreshing public keys from remote sources (compose)
  • when verifying messages (does that also update keys? I know it TOFU fetches a key but not sure if it would attempt to update existing key?)
  • when importing pubkey sent by another user (pgp_pubkey.htm)
  • when importing pubkeys in settings (contacts.htm)

All of the above places would have an if to handle this situation and not update key that's already revoked. Often this will result in a ui modal or toast message to the user if it was user-initiated action. In situations when this was an automatic action (like compose window lookups after user enters recipient), the UI interaction could be skipped and the update would be skipped silently (still in high level code).

  1. throw an error in low level function, in case we missed any edge cases. In this case, save and update function would check if existing stored key is revoked. If it is, it will throw new Error("Wrongly attempted to replace a local revoked pubkey with non-revoked version")

If the error ever throws, it will get reported to the backend, and I'll get to know about it. Then we'll have the stack trace to know which case we missed to handle in high level code.


I don't know if I'm over doing it. We could just silently skip updating in low level code and don't tell the user. But that could lead to some behavior that may appear buggy to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. My intuition was to update the schema to add this flag to storage. Then filter based on that when pulling keys out of storage.

Actually, to make a unified solution for both OpenPGP and S/MIME revocation (we do want to support CRL later on, don't we?), the easiest would be to add a flag, or keep a table of revoked keys' fingerprints.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let you choose an approach that you deem appropriate. I'd be ok to prevent updating revoked keys in a followup PR - the situation now is already better then before (it used to not be even able to import revoked keys, I think).

@lgtm-com
Copy link

lgtm-com bot commented Apr 21, 2021

This pull request introduces 1 alert when merging 570d1c5 into 8114532 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 8, 2021

This pull request introduces 1 alert when merging 444ab4a into 4868497 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@rrrooommmaaa rrrooommmaaa marked this pull request as ready for review May 8, 2021 12:49
@tomholub tomholub merged commit 8304791 into master May 8, 2021
@tomholub tomholub deleted the issue-3018-multiple-wkd-response branch May 8, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

process / update revoked and expired keys from WKD response

2 participants